Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor BaseRowItem to use inheritance #3598

Merged
merged 15 commits into from
May 25, 2024
Merged

Refactor BaseRowItem to use inheritance #3598

merged 15 commits into from
May 25, 2024

Conversation

nielsvanvelzen
Copy link
Member

@nielsvanvelzen nielsvanvelzen commented May 21, 2024 •
edited
Loading

Changes

  • Split BaseRowItem into separate classes for the various types of content
  • Remove a bunch of cruft (although not all)
  • Make BaseRowItem (mostly) immutable
  • Remove dependency injection from BaseRowItem
  • Fix various issues related to adapter mutations caused by the index being a part of the BaseRowItem

Issues

github-advanced-security[bot]
github-advanced-security bot found potential problems May 22, 2024
View reviewed changes
app/src/main/java/org/jellyfin/androidtv/ui/itemhandling/BaseRowItem.kt Fixed Show fixed Hide fixed
app/src/main/java/org/jellyfin/androidtv/ui/itemhandling/BaseRowItem.kt Fixed Show fixed Hide fixed
app/src/main/java/org/jellyfin/androidtv/ui/itemhandling/BaseItemDtoBaseRowItem.kt

else -> when {
baseItem?.criticRating != null -> when {
baseItem.criticRating!! > 59f -> R.drawable.ic_rt_fresh

Check warning

Code scanning / detekt

Report magic numbers. Magic number is a numeric literal that is not defined as a constant and hence it's unclear what the purpose of this number is. It's better to declare such numbers as constants and give them a proper name. By default, -1, 0, 1, and 2 are not considered to be magic numbers. Warning

This expression contains a magic number. Consider defining it to a well named constant.
github-advanced-security[bot]
github-advanced-security bot found potential problems May 22, 2024
View reviewed changes
app/src/main/java/org/jellyfin/androidtv/ui/itemhandling/BaseItemDtoBaseRowItem.kt
).joinToString(" - ")

val timestamp = buildString {
append(SimpleDateFormat("d MMM").format(TimeUtils.getDate(baseItem.startDate)))

Check warning

Code scanning / Android Lint

Implied locale in date format Warning

To get local formatting use getDateInstance(), getDateTimeInstance(), or getTimeInstance(), or use new SimpleDateFormat(String template, Locale locale) with for example Locale.US for ASCII dates.
github-advanced-security[bot]
github-advanced-security bot found potential problems May 23, 2024
View reviewed changes
app/src/main/java/org/jellyfin/androidtv/ui/itemhandling/BaseRowItem.kt Fixed Show fixed Hide fixed
app/src/main/java/org/jellyfin/androidtv/ui/itemhandling/BaseRowItem.kt

open class BaseRowItem protected constructor(
abstract class BaseRowItem protected constructor(

Check warning

Code scanning / detekt

Always override hashCode when you override equals. All hash-based collections depend on objects meeting the equals-contract. Two equal objects must produce the same hashcode. When inheriting equals or hashcode, override the inherited and call the super method for clarification. Warning

A class should always override hashCode when overriding equals and the other way around.
app/src/main/java/org/jellyfin/androidtv/ui/itemhandling/AudioQueueBaseRowItem.kt Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems May 23, 2024
View reviewed changes
app/src/main/java/org/jellyfin/androidtv/ui/playback/AudioNowPlayingFragment.java
mCounter.setText(((BaseRowItem) item).getIndex() + 1 + " | " + mQueueRow.getAdapter().size());
// Keep counter
ItemRowAdapter adapter = (ItemRowAdapter) mQueueRow.getAdapter();
mCounter.setText((adapter.indexOf(item) + 1) + " | " + adapter.size());

Check warning

Code scanning / Android Lint

TextView Internationalization Warning

Do not concatenate text displayed with setText. Use resource string with placeholders.
app/src/main/java/org/jellyfin/androidtv/ui/playback/AudioNowPlayingFragment.java
mCounter.setText(((BaseRowItem) item).getIndex() + 1 + " | " + mQueueRow.getAdapter().size());
// Keep counter
ItemRowAdapter adapter = (ItemRowAdapter) mQueueRow.getAdapter();
mCounter.setText((adapter.indexOf(item) + 1) + " | " + adapter.size());

Check warning

Code scanning / Android Lint

TextView Internationalization Warning

Do not concatenate text displayed with setText. Use resource string with placeholders.
app/src/main/java/org/jellyfin/androidtv/ui/itemhandling/ItemLauncher.java
@@ -68,12 +66,12 @@
}
}

public void launch(final BaseRowItem rowItem, ItemRowAdapter adapter, int pos, final Context context) {
public void launch(final BaseRowItem rowItem, ItemRowAdapter adapter, final Context context) {

Check notice

Code scanning / Android Lint

Unknown nullness Note

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations
app/src/main/java/org/jellyfin/androidtv/ui/itemhandling/ItemLauncher.java
@@ -68,12 +66,12 @@
}
}

public void launch(final BaseRowItem rowItem, ItemRowAdapter adapter, int pos, final Context context) {
public void launch(final BaseRowItem rowItem, ItemRowAdapter adapter, final Context context) {

Check notice

Code scanning / Android Lint

Unknown nullness Note

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations
app/src/main/java/org/jellyfin/androidtv/ui/itemhandling/ItemLauncher.java
@@ -68,12 +66,12 @@
}
}

public void launch(final BaseRowItem rowItem, ItemRowAdapter adapter, int pos, final Context context) {
public void launch(final BaseRowItem rowItem, ItemRowAdapter adapter, final Context context) {

Check notice

Code scanning / Android Lint

Unknown nullness Note

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations
github-advanced-security[bot]
github-advanced-security bot found potential problems May 23, 2024
View reviewed changes
app/src/main/java/org/jellyfin/androidtv/ui/itemhandling/ChapterItemInfoBaseRowItem.kt
override fun getFullName(context: Context) = chapterInfo.name
override fun getName(context: Context) = chapterInfo.name

override fun getSubText(context: Context) =

Check notice

Code scanning / Android Lint

Unknown nullness Note

Should explicitly declare type here since implicit type does not specify nullness
@nielsvanvelzen nielsvanvelzen added bug Something isn't working refactor Improvements to code realiability, readability and quality labels May 25, 2024
@nielsvanvelzen nielsvanvelzen added this to the v0.17.0 milestone May 25, 2024
@nielsvanvelzen nielsvanvelzen marked this pull request as ready for review May 25, 2024 11:58
@nielsvanvelzen nielsvanvelzen requested a review from crobibero May 25, 2024 11:58
github-advanced-security[bot]
github-advanced-security bot found potential problems May 25, 2024
View reviewed changes
app/src/main/java/org/jellyfin/androidtv/ui/home/HomeFragmentHelper.kt
@@ -44,7 +44,7 @@
mediaTypes = includeMediaTypes,
)

return HomeFragmentBrowseRowDefRow(BrowseRowDef(title, query, 0, false, true, arrayOf(ChangeTriggerType.VideoQueueChange, ChangeTriggerType.TvPlayback, ChangeTriggerType.MoviePlayback)))
return HomeFragmentBrowseRowDefRow(BrowseRowDef(title, query, 0, false, true, arrayOf(ChangeTriggerType.TvPlayback, ChangeTriggerType.MoviePlayback)))

Check warning

Code scanning / detekt

Line detected, which is longer than the defined maximum line length in the code style. Warning

Line detected, which is longer than the defined maximum line length in the code style.
@nielsvanvelzen nielsvanvelzen merged commit feafdf2 into jellyfin:master May 25, 2024
6 checks passed
@nielsvanvelzen nielsvanvelzen deleted the sdk-base-row-item branch May 25, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crobibero crobibero crobibero approved these changes

Assignees
No one assigned
Labels
bug Something isn't working refactor Improvements to code realiability, readability and quality
Projects
None yet
Milestone
v0.17.0
Development

Successfully merging this pull request may close these issues.
2 participants
@nielsvanvelzen @crobibero